-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GraphQL v16 compatibility #140
Conversation
Is there anything missing to get this merged? |
Sorry it is my bad :( Could you run it again? @boopathi |
I hate to ask but I need another :) Is it possible to make this automatic only for this PR ? @boopathi |
I removed Node 10 because v16 doesn't support it and it has been no longer maintained. @boopathi That's why CI failed :/ |
@ardatan switch from node 15 to node 16 on the build. |
@ruiaraujo @boopathi @topicus can you please review this PR? seems like it's breaking on other tools as well ( |
@ruiaraujo Done! |
@ruiaraujo I also ran prettier and ts-lint to fix format/lint issues. |
@ardatan the build is still complaining about the formatter. |
@ruiaraujo Prettier's TSLint plugin fails because TSLint doesn't know about the following syntax;
While we need the syntax above for TS v4 I can do that in this PR? |
@ardatan yes |
@ardatan we also need to conver https://github.com/zalando-incubator/graphql-jit/blob/master/tslint.json to eslint.json |
e60500e
to
255d5d8
Compare
I think rules are fine now since TSLint rules don't exist as-is in ESLint. |
@ardatan almost there, you're getting closer. 😄 |
I hope this time it will pass @ruiaraujo :) |
👍 |
I think you need to update repo settings for "Expected" workflows |
@ardatan we will do it when we are ready to merge with the second review, no worries. Thanks for the great contribuition. |
5daa5cd
to
7287de6
Compare
7287de6
to
41c619d
Compare
👍 |
@ruiaraujo @boopathi @topicus @addityasingh When could this get merged and released? We really need this for our libraries in order to upgrade to GraphQL v16 :/ |
package.json
Outdated
"jest": "^24.7.1", | ||
"lint-staged": "^8.1.5", | ||
"prettier": "^1.19.1", | ||
"prettier": "^2.4.1", | ||
"ts-jest": "^24.0.2", | ||
"ts-node": "^8.0.3", | ||
"tslint": "^5.15.0", | ||
"tslint-config-prettier": "^1.18.0", | ||
"tslint-plugin-prettier": "^2.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will tslint
dependencies still be needed even if we move to eslint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep it open until the change is pushed, for visibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad :) I forgot to push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you take a look if it is good for you? @oporkka
👍 |
1 similar comment
👍 |
Thank you <3 |
GraphQL v16 has been released recently so this PR does the following changes for the compatibility;
collectFields
injson.ts
because it has been changed in v16toEqual
withtoMatchObject
to compare results becauseGraphQLError
is no longer serialized like JSO